Skip to content

obsidian: use the bundled Electron in Obsidian binary instead of from nixpkgs#519466

Closed
hesprs wants to merge 1 commit into
NixOS:masterfrom
hesprs:master
Closed

obsidian: use the bundled Electron in Obsidian binary instead of from nixpkgs#519466
hesprs wants to merge 1 commit into
NixOS:masterfrom
hesprs:master

Conversation

@hesprs

@hesprs hesprs commented May 12, 2026

Copy link
Copy Markdown

Obsidian's Nix derivation fetches the compiled Obsidian tarball from Obsidian GitHub release. The fetched tarball already has Electron bundled.

Original derivation enforces Obsidian to use electron_39 from Nixpkgs instead of the one comes with it. Obsidian team seems to have patched Electron itself in the updates between v1.11.4 and v1.12.7. Using electron_39 in Nixpkgs invalidates the patch and causes certain bugs. A known one is https://forum.obsidian.md/t/keyboard-triggered-context-menu-does-not-show-spell-suggestions/114141

This commit removes electron_39 and _7zz dependency, making Obsidian depend on the Electron that is complied with it.

This commit also attempts to improve Electron's secret store auto-detection. The original detection fails on non-KDE or non-Gnome environments. A wrapper script is added to detect the currently running secret service and set the secret store backend flag instead of relying on matching user's desktop environment. This issue has already been improved in Electron 42: electron/electron#49054. But since Obsidian uses Obsidian 39, this patch is still needed.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

… nixpkgs

Obsidian's Nix derivation fetches the compiled Obsidian tarball from Obsidian GitHub release. The fetched tarball already has Electron bundled.

Original derivation enforces Obsidian to use electron_39 from Nixpkgs instead of the one comes with it. Obsidian team seems to have patched Electron itself in the updates between v1.11.4 and v1.12.7. Using electron_39 in Nixpkgs invalidates the patch and causes certain bugs. A known one is https://forum.obsidian.md/t/keyboard-triggered-context-menu-does-not-show-spell-suggestions/114141

This commit removes electron_39 and _7zz dependency, making Obsidian depend on the Electron that is complied with it.

This commit also attempts to improve Electron's secret store auto-detection. The original detection fails on non-KDE or non-Gnome environments. A wrapper script is added to detect the currently running secret service and set the secret store backend flag instead of relying on matching user's desktop environment. This issue has already been improved in Electron 42: electron/electron#49054. But since Obsidian uses Obsidian 39, this patch is still needed.
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle! labels May 12, 2026
@w-lfchen

Copy link
Copy Markdown
Member

what is the difference between this pr and #519461?

@hesprs

hesprs commented May 12, 2026

Copy link
Copy Markdown
Author

That PR fails the commit message lint check and contains multiple commits. This PR has only one done cleanly.

@zaninime

Copy link
Copy Markdown
Contributor

I understand this PR fixes a real problem, however I'm not sure how I feel about switching to a binary distribution of Electron (that is being binary-patched anyway, by the derivation), instead of relying on the one built by Hydra.

A known one is https://forum.obsidian.md/t/keyboard-triggered-context-menu-does-not-show-spell-suggestions/114141

I read through the discussion, but I haven't seen any mentions made by the maintainers about custom patches made on Electron. While Electron is MIT-licensed (hence Obsidian's team is not forced to release the source code with their modifications) I am sure they have interest in upstreaming any improvements they make to it.

Have you tried keeping the other changes you made in this PR, but keeping electron_39?

@zaninime

Copy link
Copy Markdown
Contributor

Also: there's no need to open a new PR every time you want to change something, or a check is failing. Just amend the commits.

@hesprs

hesprs commented May 13, 2026

Copy link
Copy Markdown
Author

but I haven't seen any mentions made by the maintainers about custom patches made on Electron.

I made this conclusion by doing experiments and control variables. The experiments are:

  1. use current nixpkgs-unstable: spell check fails inconsistently
  2. use nixpkgs-25.11: spell check works
  3. use AppImage of the latest version of Obsidian: spell check works
  4. use the derivation in this PR: spell check works

Variables controlled:

  1. system environment
  2. (in experiment 1 and 2) electron version. The version of electron_39 are the same 39.8.9
  3. (in experiment 1 and 2) derivation script. The script only varies slightly about Obsidian CLI between unstable and 25.11, doesn't affect Obsidian Electron APP.

Deduction:

  1. near everything was the same in 2 and 4, but the bug disappears in 4.
  2. the only difference between 2 and 4 (or 2 and 3) is the Electron used.
  3. Electron doesn't change in 1 and 2, but the bug appears in 2.

Obviously, Electron itself is changed (I guess it was a patch by Obsidian team) between 1 and 2.

I am sure they have interest in upstreaming any improvements they make to it.

Maybe we can try, but I think is less feasible to ask a proprietary software team about this.

Have you tried keeping the other changes you made in this PR, but keeping electron_39?

I used to use an overlay: https://forum.obsidian.md/t/how-does-obsidian-linux-find-system-secret-storage/113465/2

Secret store worked, spell check still failed.

Also: there's no need to open a new PR every time you want to change something, or a check is failing. Just amend the commits.

My bad, I'm not so skillful at rewriting Git history though.

@w-lfchen w-lfchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enforces Obsidian to use electron_39

you can override it

Obsidian team seems to have patched Electron itself in the updates between v1.11.4 and v1.12.7

proof?

This commit also attempts to improve Electron's secret store auto-detection.

this should be done in a separate commit, maybe even a separate pr.
and shouldn't this be directed at electron?

using obsidian's electron invalidates any nix-specific patches to electron. i am opposed to doing that.

imagemagick
];

buildInputs = with pkgs; [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are in nixpkgs, using pkgs anywhere is just wrong. he derivation is passed to callPackage, which resolves the arguments.

your entire derivation is full of this pattern, and needs to be rewritten without it.

cp -a ./* $out/share/obsidian/

OBSIDIAN_BIN="$out/share/obsidian/obsidian"
chmod +x "$OBSIDIAN_BIN"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the binary already be executable?

@w-lfchen

Copy link
Copy Markdown
Member

also, your commit description is not long enough. it is clearly missing a recipe for a delicious chocolate cake, please amend the commit accordingly.

@hesprs

hesprs commented May 13, 2026

Copy link
Copy Markdown
Author

I just figured out everything. The bug that I described is not caused by any "patch" from Obsidian team. It is caused by nixpkgs-unstable update timing. Last time I update my flakes was last month, at that time, the Obsidian derivation was:

  fetchurl,
  lib,
  makeWrapper,
  electron,
  makeDesktopItem,
  imagemagick,
  autoPatchelfHook,
  writeScript,
  _7zz,
  commandLineArgs ? "",
}:

electron defaults to Electron 41 while Obsidian is using 39. Causing severe version mismatch.

In #510075, Electron was pinned at 39. But since that happened after my update, my version stopped at the broken 41 version. I didn't know the update and assumed I was seeing the bug with Electron 39 when making this PR.

The correct fix is simply updating my flakes.

I feel extremely sorry about the back and forth and my unreasonable allegations. Please forgive my ignorance and arrogance during the discussion on this issue. I will close this PR right now and there won't be more PRs about this.

@hesprs hesprs closed this May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants